[#12048] Migrate tests for CalculateUsageStatisticsAction #13247
Conversation
| Instant endTime = TimeHelper.getInstantNearestHourBefore(Instant.now()); | ||
| Instant startTime = endTime.minus(COLLECTION_TIME_PERIOD, ChronoUnit.MINUTES); | ||
| UsageStatistics testUsageStatistics; | ||
| UsageStatisticsAttributes testUsageStatisticsAttributes; |
There was a problem hiding this comment.
let's make these attributes private for consistency
| testUsageStatistics = new UsageStatistics( | ||
| startTime, | ||
| COLLECTION_TIME_PERIOD, | ||
| NUMBER_OF_RESPONSES, | ||
| NUMBER_OF_COURSES, | ||
| NUMBER_OF_STUDENTS, | ||
| NUMBER_OF_INSTRUCTORS, | ||
| NUMBER_OF_ACCOUNT_REQUESTS, | ||
| 0, | ||
| 0); |
There was a problem hiding this comment.
I think we can create a getTypicalUsageStatistics() method in BaseTestCase.java with all of these values/attributes over there for extensibility.
| testUsageStatisticsAttributes = | ||
| UsageStatisticsAttributes.builder(startTime, COLLECTION_TIME_PERIOD) | ||
| .withNumResponses(NUMBER_OF_RESPONSES) | ||
| .withNumCourses(NUMBER_OF_COURSES) | ||
| .withNumStudents(NUMBER_OF_STUDENTS) | ||
| .withNumInstructors(NUMBER_OF_INSTRUCTORS) | ||
| .withNumAccountRequests(NUMBER_OF_ACCOUNT_REQUESTS) | ||
| .build(); |
There was a problem hiding this comment.
In UsageStatisticsAttributes.java, there is a valueOf() method that takes in a teammates.storage.entity.UsageStatistics and creates a UsageStatisticsAttributes with the same attributes. It's quite similar to what we're trying to do here
I think we can create another valueOf (Overloading) for the SQL version of UsageStatistics. To resolve the class name conflicts, the new method's parameter type can be teammates.sqlstorage.entity.UsageStatistics (No need to import). The old method can remain.
@InfinityTwo Let me know what you think about this proposal!
There was a problem hiding this comment.
I think the idea is great, but from what I saw the last time, the use of UsageStatisticsAttributes is only for the old datastore logic. I think once migration is completed, I believe the use for it becomes redundant and it solely uses UsageStatistics in the action, assuming we remember to change it after migration. Right now, it seems like it just concatenates both values together for the output.
I was thinking we could add a comment to remove it eventually but @jasonqiu212 what do you think? I'm cool with either ways.
Part of #12048
Outline of Solution
Added the unit tests for CalculateUsageStatisticsAction for PostgreSQL.